-
Notifications
You must be signed in to change notification settings - Fork 56
chore: Add a high level direction for improving device and feature discovery #722
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…scovery This is meant to try to summarize the device features & discovery pieces that need to be improved to get to the next level of home assistant quality.
| | :--- | :--- | :--- | | ||
| | **Startup** | Waits for all devices to refresh. | Starts immediately after getting device list. | | ||
| | **Offline Device** | May cause setup failure or missing entities. | Device and entities are created as "Unavailable". | | ||
| | **Unknown Device** | Might crash or fail integration setup. | Gracefully handled using basic generic traits. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by this i assume you mean unknown as in devices we have never seen - rather than 'new' devices i.e. the b01 devices
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant here an unsupported device. There was a bug where it would throw an exception and fail the whole integration, even if some devices were supported. I believe this may now be fixed though.
| - **Result**: The device is registered and visible in the UI immediately. | ||
| 3. **Connection & Recovery (Background)**: | ||
| - In the background, `DeviceManager` works to establish MQTT/Local connections. | ||
| - As devices come online, the `DeviceListener` triggers updates, and entities become "Available". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, DeviceListener utilizes the existing RoborockProtocol to automatically listen for devices.
Devices will ping us with their ip and their duid. We can then automatically add them.
Some instances will have these ports blocked (we should recommend opening them on the docs). If the ports are blocked, we should do a less often Home data call.
|
|
||
| To verify a device is supported, we'll need to move to the model of checking device features, or only relying on the product information `HomeData` instead of waiting for a live response. | ||
|
|
||
| #### Entity Registration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While less sophisticated, I wonder if utilizing the HA entity manager would be easier - to determine what entities have existed before
|
|
||
| #### Entity Registration | ||
| - **Current Model**: We try to communicate with the device. If it fails, we don't know what sensors to create, so we create nothing. | ||
| - **Proposed Model**: We check the account metadata or cached device features, and create the corresponding sensors immediately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is a new device that we fail to communicate with - do we still create the device and just add no entities?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that would be the proposal.
| - *Rationale*: A timeout or connection failure is a *device state*, not a discovery failure. Once we get the list (from cache or cloud), discovery is successful. | ||
| - Update `DeviceManager` to optionally *not* auto-connect. The Home Assistant Coordinator will be responsible for calling `start_connect()` so that disabled devices remain disconnected. | ||
| - Ensure `device_ready` callbacks fire once feature discovery completes even if the initial connection fails | ||
| - Add a periodic task to re-run `discover_devices` every N minutes to fetch fresh `HomeData`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be careful calling home_data often. This is a potential route to rate limiting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to propose a frequency? We could also do this:
(1) Never poll in the background
(2) Fetch on startup only if over X hours since last fetch (e.g. if you reload many times in a row it does nothing)
|
I wonder if we should follow this rule: If a feature is supported on only some devices - it should be split up into a separate trait. This may mean that things like 'Status' need to be split up. It is a lot easier for us to check/organize the logic if it were checking one trait rather than checking the attribute of a trait. But that may have some complexity to it. What did you have in mind for this part of the system? The actual entity/feature mapping. |
This sounds like a reasonable proposal to me. |
By this i mean: It sounds like it can work, i don't have any objection to it, and i don't have any other better suggestions. I like the idea of treating "device features" as an internal implementation detail. |
Although - some devices have more data for things like dnd than others. and we may end up with something like DnDTrait Then consumables as well can be different per devices, meaning we may need to have a seperate trait per consumable. I guess this may get pretty big pretty fast. What about a custom decorator for properties? If no decorator -> if the device ahs the trait it is supported. If decorator -> device must have specific trait(s) for this property to be supported. We can still abstract this up somehow, but then device features stay grouped with the specific thing that they are supporting. |
What would the caller API be to check if a feature is supported? An alternative I can think of is to have an enum with supported features on the trait (an enum value per optional field) and the convention would just be that it has a similar name to the specific fields. |
theoretically, it could just hit the property and get back some exception? Not sure. Or the property could dynamically pass the information upwards and create a kind of device feature manager. Or the property could set some value on the trait that we can then check.
That could be fine. entities would end up having a extra field on the description of which enum it needs for that entity to exist. How do you then map that enum to the actual device feature? i.e. how do we know that this device supports that enum value for that property? I think device features is likely too abstract of a concept to expose to HA, so I like where we are heading with this. |
|
Okay my thoughts: device features is currently too granular of a concept to expose heavily outside of the library. It makes sense to have and to have as it is now imo - we should aim for it to have as much parity with the reverse engineered code as possible. That way it is easy to add new things. What we need is a step above device features that utilizes the device features to cleanly state supported features. Sometimes for support it may need to just use one device feature key, sometimes it may be multiple conditionals. sometimes it may be one and a specific dock type, etc. In my mind there are a few places this could end up.
side_brush_work_time would be a field with a bunch of keys in the metadata that declare what is needed for support. I really don't know. there are a bunch of different approaches I can think of but none of them really feel right to me. What I do know is I want to hide the complexity on the library side and make it as easy as possible to just basically say 'is parameter X of trait Y supported for my device'. I don't want the caller to be responsible for passing around the arguments that it needs or knowing the specific complicated device features or dock types, etc. |
|
The points I am hearing that resonate with me are, that I think can be "decisions" we agree on:
if you agree with that, as a next step we can try a couple specifics like what you are proposing and see if we like them. Your proposed API makes sense, so a prototype is probably in order. I would say we try to use this to identify the existing optional sensors / binary sensors in home assistant (mop, cleaning area, dock entity stuff) and see if we can get something to make sense. |
This is meant to try to summarize the device features progress and potential next steps, and discovery pieces that need to be improved to get to the next level of home assistant quality.